Address security issues in the signup module#667
Open
vagisha wants to merge 8 commits into
Open
Conversation
… that carries AdminPermission (admin rights) in any subfolder of the project, not only at the project root. Co-Authored-By: Claude <noreply@anthropic.com>
* SignUpApiAction and BeginAction now email the owner of an already-registered address (new sendExistingAccountEmail helper) instead of doing nothing. Both signup paths now send an email, so response timing no longer reveals whether an account exists. * The existing account is never modified, and send failures are logged and swallowed so the response stays identical to a new signup. The email points the owner to the "Forgot your password?" reset flow. * Renamed the SignUpApiAction success status from USER_ADDED to SUCCESS, which was misleading in the existing-account case where no user is added. The external signup wiki form must key its success branch off SUCCESS. Co-Authored-By: Claude <noreply@anthropic.com>
…ion status * ChangeGroupsApiAction returns TARGET_NOT_ALLOWED when validateGroupChangeTarget refuses a privileged, site, or cross-project target. NO_PERMISSIONS still covers callers that are ineligible for the transition. * Added SignUpGroupChangeSecurityTest. As a non-admin it plants each dangerous transition rule, then verifies the move is refused and membership unchanged, while a plain same-project move still succeeds. Co-Authored-By: Claude <noreply@anthropic.com>
…e group-change test * SignUpApiAction and BeginAction now run the existing-account and new-account branches through one try/catch, so a mail-send failure returns the same generic error for both. * SignUpGroupChangeSecurityTest now verifies through the auditLog schema that a successful self-service group change adds one "Client API Actions" event and a refused move adds none, comparing counts before and after each move since the event is recorded in the root container that outlives project cleanup. See ai/todos/active/TODO-LK-20260723_signup-security.md Co-Authored-By: Claude <noreply@anthropic.com>
* SignUpApiAction and BeginAction also catch SQLException from the new-account insert, so a DB failure returns the same generic error as any other and cannot reveal whether the account existed. * validateGroupChangeTarget drops the workbook-only container filter and its stale comment. The admin check is unchanged. * RemoveGroupChangeProperty now guards against a null rule map and removes the entry with the correct string key. Group-change audit events log group names instead of bare ids. * SignUpGroupChangeSecurityTest adds a case for a target that inherits admin via membership in an admin group. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens the signup module by preventing privilege escalation through self-service group changes, making signup responses non-enumerable, improving audit logging of admin configuration changes, and ensuring mail-send failures don’t leak internal details to callers.
Changes:
- Adds server-side validation for self-service group transitions to block moves into site groups, cross-project groups, or any group with administrative permission anywhere in the project tree; successful moves are now audited.
- Makes signup responses uniform whether or not the email already has an account (and logs internal mail errors while returning a generic message).
- Adds an integration test (
SignUpGroupChangeSecurityTest) that verifies group-change validation and audit behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| signup/src/org/labkey/signup/SignUpController.java | Adds audit logging, uniform signup behavior, and target validation for self-service group moves. |
| signup/test/src/org/labkey/test/tests/signup/SignUpGroupChangeSecurityTest.java | New test covering group-change security validation and auditing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
vagisha
marked this pull request as ready for review
July 26, 2026 22:56
…dit message, matching the admin-config audit events
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale
This PR addresses the security issues found in the signup module following an internal review. It tightens validation on self-service group changes, makes signup responses consistent, improves auditing of administrative changes, and avoids surfacing internal mail errors to callers.
Security review findings (signup module)
The review flagged six items in the signup module. Four are addressed by this PR; the two rate-limiting items were already handled by the kaptcha work in #638.
Related Pull Requests
Changes
SignUpGroupChangeSecurityTest, covering the group-change validation and its auditing.Deployment note. Two DB-stored wiki pages on the skyline website need updating after deployment: the signup form should branch on
status == 'SUCCESS', and the Skyline-daily register form should handle theTARGET_NOT_ALLOWEDstatus returned by a rejected group change (plus a fallback for any unrecognized status).Co-Authored-By: Claude noreply@anthropic.com